Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix matching codecs with different rate or channels #3021

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aler9
Copy link
Member

@aler9 aler9 commented Jan 25, 2025

Description

In pion/webrtc, currently codecs are matched regardless of the clock rate and the channel count, and this makes impossible to fully support codecs that might have a clock rate or channel count different than the default one, in particular LPCM, PCMU, PCMA and multiopus (the last one is a custom Opus variant present in the Chrome source code to support multichannel Opus).

For instance, let's suppose a peer (receiver) wants to receive an audio track encoded with LPCM, 48khz sample rate and 2 channels. This receiver doesn't know the audio codec yet, therefore it advertises all supported sample rates in the SDP:

LPCM/44100
LPCM/48000
LPCM/44100/2
LPCM/48000/2

The other peer (sender) receives the SDP, but since the clock rate and channel count are not taken into consideration when matching codecs, the sender codec LPCM/48000/2 is wrongly associated with the receiver codec LPCM/44100. The result is that the audio track cannot be decoded correctly from the receiver side.

This patch fixes the issue and has been running smoothly in MediaMTX for almost a year.

Unfortunately, in lots of examples and tests, clock rate and/or channels are not present (and in fact they are producing horrible SDPs that contain VP8/0 instead of VP8/90000 and are incompatible with lots of servers) therefore this new check causes troubles in existing code. In order to maintain compatibility, default clock rates and channels are provided for most codecs.

In the future, it might be better to update examples (i can do it in a future patch) and remove the exception.

@aler9 aler9 force-pushed the patch-codecs-match branch from a488626 to 0e8d4ec Compare January 25, 2025 13:46
Copy link

codecov bot commented Jan 25, 2025

Codecov Report

Attention: Patch coverage is 96.80851% with 3 lines in your changes missing coverage. Please review.

Project coverage is 78.25%. Comparing base (70d06fd) to head (8eda30b).

Files with missing lines Patch % Lines
internal/fmtp/fmtp.go 95.38% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3021      +/-   ##
==========================================
+ Coverage   78.22%   78.25%   +0.02%     
==========================================
  Files          91       91              
  Lines       11159    11234      +75     
==========================================
+ Hits         8729     8791      +62     
- Misses       1940     1950      +10     
- Partials      490      493       +3     
Flag Coverage Δ
go 80.11% <96.80%> (+0.01%) ⬆️
wasm 63.44% <73.80%> (+0.22%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aler9 aler9 force-pushed the patch-codecs-match branch 7 times, most recently from 54a37a9 to eb71ae1 Compare January 25, 2025 15:35
@aler9 aler9 changed the title Fix codec matching in case of different clock rate or channels Fix codec matching with different rate or channels Jan 25, 2025
@aler9 aler9 force-pushed the patch-codecs-match branch from eb71ae1 to 55b280c Compare January 25, 2025 15:36
@aler9 aler9 changed the title Fix codec matching with different rate or channels Fix matching codecs with different rate or channels Jan 25, 2025
@aler9 aler9 force-pushed the patch-codecs-match branch 2 times, most recently from c10ed6e to b31dde1 Compare January 25, 2025 15:39
@aler9 aler9 requested a review from JoeTurki January 25, 2025 15:45
Copy link
Member

@JoeTurki JoeTurki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for this! Just few random thoughts; although I'm not fully confident with the Pion codebase yet to approve this so you'll have to wait for @Sean-Der , this change does make sense to me.

Thank you again <3

internal/fmtp/fmtp.go Show resolved Hide resolved
internal/fmtp/fmtp.go Outdated Show resolved Hide resolved
rtpsender_test.go Outdated Show resolved Hide resolved
track_local_static_test.go Outdated Show resolved Hide resolved
@JoeTurki JoeTurki requested a review from Sean-Der January 25, 2025 22:19
@aler9
Copy link
Member Author

aler9 commented Feb 10, 2025

@JoeTurki thank you very much for the detailed review. I'm gonna rewrite this in order to provide defaults for every supported codec and support any existing situation, and at the same time solve the original issue that led to this PR (that was the fact that LPCM/48000/2 was wrongfully associated with LPCM/44100/1, leading to corrupted audio).

@aler9 aler9 force-pushed the patch-codecs-match branch 10 times, most recently from 4ef7239 to aa27913 Compare February 10, 2025 13:24
@aler9
Copy link
Member Author

aler9 commented Feb 10, 2025

@JoeTurki i added defaults for every codec that i know of, restored existing tests and simplified the whole thing.
In my opinion this is ready to be merged.
I'll wait a final opinion by @Sean-Der anyway.

@SijmenHuizenga
Copy link

Thanks for this pr! I just tested this branch together with livekit-go-sdk v2.4.2 inside MediaMTX v1.11.3 and it works great 👍

(It solves a problem we were seeing where Livekit was not accepting streams when compiled with the MediaMTX fork of pion/webrtc)

@JoeTurki
Copy link
Member

@aler9 Thank you so much <3

@Sean-Der
Copy link
Member

@aler9 @JoeTurki no need to wait on me! As long as we don't break existing code push/tag whatever you need for forward progress :)

@aler9 aler9 force-pushed the patch-codecs-match branch 2 times, most recently from 28ba39d to e5cb963 Compare February 12, 2025 21:16
@Sean-Der Sean-Der added this to the V4.1.0 milestone Feb 13, 2025
Consider clock rate and channels when matching codecs. This allows to
support codecs with the same MIME type but sample rate or channel count
that might be different from the default ones, like PCMU, PCMA, LPCM
and multiopus.
@aler9 aler9 force-pushed the patch-codecs-match branch from e5cb963 to 8eda30b Compare February 13, 2025 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants